feat: pay bolt11 Lightning invoices via /api/v1/send - #72

BackendBanda Code Review — PR #72: bolt11 Lightning invoice payments

Review effort: 4/5 | Reviewer: backend-banda | Date: 2026-10-14

Summary

Solid, well-structured feature. The core correctness concerns (double-spend, false settlement confirmation, replay attacks) are all addressed. A few things need fixing or a decision before merge.

✅ What's good

HMAC single-use enforcement (middleware.go)

• signatureSet.claim() with in-memory map + TTL = correct approach. Eviction on each call avoids unbounded growth. The TTL is 2 * tolerance seconds — wide enough to cover the full valid window.
• Future-timestamp rejection (age < -tolerance) closes the captured-request replay window. Breaking change called out clearly in PR description.

Settlement polling (settlement.go)

• WaitForOutgoingPayment → polling loop with exponential backoff (500ms → 3s cap × 1.5). Correctly stays inside the 90s write timeout via settlementBudget trimming against elapsed time since request start.
• classifyPayment is appropriately conservative: paid:false with no status → PaymentStateUnknown, never failure. This avoids falsely telling callers their sats came back.
• Settlement preimage zero-value guard (strings.Trim(preimage, "0") == "") is correct — lnbits returns all-zero preimage for unsettled payments.
• Test coverage of all lnbits response shapes including public-key response (no status) → PaymentStateUnknown. Good.

Balance reservation under concurrency (balance_reservation.go)

• Per-sender sync.Mutex serialises the check-then-reserve step. once.Do in Release() prevents double-release. The concurrent stress test (50 goroutines × 100 sat from 1000 balance → exactly 10 granted) is clean and passes.
• Reservation released immediately after lnbits.Pay accepts, not after settlement polling: correct — lnbits already reflects the deduction once accepted, so holding longer would double-count.

Idempotency (two-layer)

• Short MemoCache lock (concurrent requests, ~5 min) + durable InvoiceApprovalLock in Bunt (24h, for re-submitted approvals) — the gap between them is documented and addressed.
• Invoice payment hash validated as ^[0-9a-f]{64}$ before use in lnbits URL. ✅

Approval flow (api_approval.go)

• Per-request apiApprovalMarkup avoids the race between concurrent approval messages overwriting shared callback data.
• Expiry check at approve-time (time.Now().After(approvalData.expiresAt())) with backwards-compat fallback for old approvals (CreatedAt + PendingTxExpiry). Correct.
• Failed Telegram delivery → immediate error + rollback (abandonPendingTransaction), not a silent 202. ✅

🔴 Must-fix before merge

1. signatureSet is process-local — multi-instance deployments allow replay

var usedSignatures = &signatureSet{...} lives in Go process memory. If the bot runs as more than one instance (e.g. two pods, or a restart mid-tolerance-window), a captured signature can be replayed against the second instance.

For a single-process bot this is fine; for any scaled deployment it is not. Either:

• Document explicitly that the bot must run as a single instance, and add a startup log warning if that changes, OR
• Move the seen-signature store to Bunt (same pattern as InvoiceApprovalLock) with a TTL-based cleanup.

Action needed: at minimum, a comment in signatureSet stating the single-instance assumption. If multi-instance is in the roadmap, make it Bunt-backed now.

2. MemoCache TTL for the concurrent invoice lock is undocumented / unclear

s.MemoCache.SetNX(lockKey, "locked") is used without a TTL here (in sendToInvoice). The comment says "short lock...expires after 5 minutes" but I don't see where that TTL is set. If MemoCache entries never expire, the lock is permanent: a request that dies after SetNX but before defer Delete (e.g. server crash, panic) would permanently block that invoice from being paid.

Please confirm: does MemoCache have a global TTL? If not, add an explicit TTL to this SetNX call (e.g. 10 minutes), or document the GC/restart recovery path.

3. SendPaymentStatus (GET /api/v1/send/payment/{hash}) has a zero-timeout single lookup

settlement := s.Bot.Client.WaitForOutgoingPayment(*fromUser.Wallet, paymentHash, 0)

With timeout=0, WaitForOutgoingPayment does at most one rate-limited check. If lnbits is slow or the rate limiter is busy, this could return PaymentStateUnknown for a payment that would resolve within 1s. The caller gets unknown and has to retry anyway — so the endpoint is not saving them a round trip.

Suggest: a short timeout (e.g. 5s) so a single poll has a chance to land, while still returning promptly. Or document that callers should expect unknown and retry with backoff.

🟡 Non-blocking observations (address in follow-up or now, author's call)

4. lnbc... / lightning: prefix lowercased before invoice check

strings.ToLower(req.To) before lightning.IsInvoice — fine for the prefix, but bolt11 invoices themselves use bech32 which is case-insensitive. decodepay handles upper/lower, so this is safe. Just noting it's intentional.

5. Approval-flow invoice path: reservation acquired in approveAPITransactionHandler but not in executeApprovedInvoicePayment?

In the non-invoice approval path, ReserveBalance is called before the transfer and deferred-released. In executeApprovedInvoicePayment, I don't see an equivalent reservation — the approval handler reserves, but does it hold through the pay call? Please verify the reservation is not released before lnbits.Pay returns, or add one inside executeApprovedInvoicePayment.

6. payment_hash in pending transaction ID is included in plain text

id := fmt.Sprintf("pending-%s-invoice-%s-%d", ...) — the payment hash is part of the bunt key. Payment hashes are public on the Lightning network so this is not a secret, but if bunt keys are ever logged or exposed, it ties a username to a specific payment. Minor.

7. Test for SendPaymentStatus endpoint is missing

The settlement_test.go covers classifyPayment well. The HTTP handler itself (SendPaymentStatus) has no test. At minimum an integration-style test that exercises the auth check and the hash validation would catch regressions.

Verdict

Approve with blocking comments resolved. Issues 1 and 2 need a documented answer or a code fix; issue 3 needs at minimum a comment. Non-blocking items can go in a follow-up.

go build ./... and go test ./... are reported clean; I cannot run them locally (no repo access) but the test coverage on the new packages is solid.

⚠️ Note for merge: this PR changes the HMAC auth contract for ALL existing /send, /userbalance, /usertransactions, and status callers. Any client that retries with the same timestamp+signature will receive 401 Request already processed. Make sure existing integrations (CeyFi, DCA bot) sign fresh timestamps on every request including retries before merging to main.

— backend-banda (T-0025)